PR #2052 — Orchestrator E2E Tests: Review

Summary

This PR adds a comprehensive E2E test suite for the Orchestrator plugin covering workflow execution, entity-workflow integration, RBAC permission matrix, and token propagation API tests. The test coverage is thorough and the infrastructure deployment logic (workflow-deployment-helpers.ts) is well-engineered with good error handling and retry patterns.

However, the test code has significant structural issues that will impact maintainability, CI reliability, and readability. The spec files are the largest in the repository, don't follow the Page Object Model used by other workspaces (RBAC, Keycloak, Notifications), contain heavy code duplication, and use retries: 0 overrides that make CI runs fragile on an inherently flaky infrastructure (OpenShift + SonataFlow + Keycloak).

Below are the findings, organized by severity.

HIGH — Architecture / Design

1. No Page Object Model for orchestrator-specific UI interactions

Both spec files interact with the Orchestrator UI extensively via raw page.getByRole(), page.getByText() scattered throughout tests. The repository has a clear precedent for the Page Object Model — RBAC (rbac-po.ts + rbac-obj.ts), Keycloak (catalog-users-obj.ts), Notifications (notifications.ts).

The PR does use OrchestratorPage from e2e-test-utils, but only for a subset of operations. All entity-workflow interactions, RBAC verification UI flows, template launching, and breadcrumb navigation are done inline.

Examples of patterns repeated across both files that should be page object methods:

• Navigate to Orchestrator and select a workflow (~15+ occurrences)
• Verify Run button state (enabled / disabled / absent) — at least 6 occurrences with identical logic
• Template launch flow (Self-service → Choose → Fill form → Review → Create) — duplicated between orchestrator.tests.ts (lines 555-598, 707-749) and orchestrator-rbac.tests.ts (lines 903-953, 1055-1081)
• Breadcrumb navigation verification (lines 656-705)

For reference, see how the RBAC workspace handles this:

• support/pages/rbac-po.ts — Business operations (createRole, deleteRole, navigateToRBACPage)
• support/pages/rbac-obj.ts — Locator factories (component library)
• support/pages/rbac.ts — Static data (column names, cell identifiers)

2. Spec files are the largest in the repository

The repo's pattern is clear: when a spec exceeds ~200-300 lines, supporting page objects or helpers keep it maintainable. For comparison:

• Tech-radar: 49 lines (simple plugin, inline helpers)
• ArgoCD: 212 lines (medium, separate helper file)
• RBAC: 637 lines (complex, full page object model)

3. Setup steps masquerading as tests → forces serial → forces retries: 0

This is the most impactful design issue. Each RBAC block follows this pattern:

test.describe.serial("RBAC: Global Workflow Access", () => {
  test.describe.configure({ retries: 0 });

  test("Create role with policies", async () => { /* setup */ });
  test("Verify role exists via API", async () => { /* setup validation */ });
  test("Verify UI behavior", async () => { /* actual test */ });
});

The first two "tests" are setup, not tests. This forces serial (test 3 depends on test 1), which forces retries: 0 (retries + serial produces inconsistent state).

retries: 0 is particularly harmful here. Orchestrator tests run on OpenShift clusters with SonataFlow, data-index services, Keycloak, and multiple operators — an inherently flaky environment. By disabling retries, a single transient cluster hiccup fails the entire RBAC suite with no recovery.

The root cause chain:

setup-as-tests → forces serial → forces retries: 0 → fragile CI

Fix the root cause and the entire chain collapses:

// AFTER: setup where it belongs
test.describe("RBAC: Global Workflow Access", () => {
  test.beforeAll(async () => {
    await createRoleWithPolicies(apiToken, roleName, ...);
  });

  test.afterAll(async () => {
    await deleteRoleAndPolicies(apiToken, roleName);
  });

  test("Workflows visible and Run button enabled", async () => {
    // actual test
  });
});

No serial. No retries: 0. Retries work naturally. The test report shows what was actually tested, not setup bookkeeping.

4. Helper files in specs/ instead of support/

Both test-helpers.ts and workflow-deployment-helpers.ts live in tests/specs/. The repo convention (RBAC, Keycloak, Backstage, Quay) is:

e2e-tests/
├── tests/
│   ├── specs/          ← only .spec.ts files
│   └── support/        ← or at top level
│       ├── pages/      ← page objects
│       ├── utils/      ← utilities
│       └── constants/  ← test data

5. Duplicated deployment setup across both files

The test.runOnce("orchestrator-setup") block is copy-pasted across both files (identical ~12 lines). This should live in a single parent test.describe that both test suites inherit from.

Preferred approach — single spec file with parent setup:

// orchestrator.spec.ts
test.describe("Orchestrator", () => {
  test.beforeAll(async ({ rhdh }) => {
    await test.runOnce("orchestrator-setup", async () => {
      const ns = rhdh.deploymentConfig.namespace;
      await rhdh.configure({ auth: "keycloak" });
      await deploySonataflow(ns);
      process.env.SONATAFLOW_DATA_INDEX_URL = "http://...";
      try {
        await rhdh.deploy({ timeout: 900_000 });
      } catch (err) {
        logOrchestratorDeployFailureDiagnostics(ns);
        throw err;
      }
    });
  });

  test.describe("Workflow Execution", () => {
    test.beforeAll(async ({ browser }, testInfo) => {
      await ensureBaselineRole(browser, testInfo);
    });
    // ... workflow + entity-workflow tests using page objects
  });

  test.describe("RBAC", () => {
    test.beforeAll(async ({ browser }, testInfo) => {
      await removeBaselineRole(browser, testInfo);
    });
    // ... data-driven RBAC tests using page objects
  });
});

Estimated ~780 lines after page objects + data-driven RBAC — comparable to the RBAC workspace.

Fallback — if the single file is still too large after refactoring, split into function-wrapped test modules (functions nest properly under the parent describe, unlike bare imports):

// orchestrator-workflows.tests.ts
export function workflowTests() {
  test.describe("Workflow Execution", () => {
    test.beforeAll(async ({ browser }, testInfo) => {
      await ensureBaselineRole(browser, testInfo);
    });
    // ... tests
  });
}

// orchestrator-rbac.tests.ts
export function rbacTests() {
  test.describe("RBAC", () => {
    test.beforeAll(async ({ browser }, testInfo) => {
      await removeBaselineRole(browser, testInfo);
    });
    // ... tests
  });
}

// orchestrator.spec.ts
import { workflowTests } from "./orchestrator-workflows.tests.js";
import { rbacTests } from "./orchestrator-rbac.tests.js";

test.describe("Orchestrator", () => {
  test.beforeAll(async ({ rhdh }) => {
    await test.runOnce("orchestrator-setup", async () => {
      // shared deployment — single place, no duplication
    });
  });

  workflowTests();  // nests under parent — inherits beforeAll
  rbacTests();      // nests under parent — inherits beforeAll
});

MEDIUM — Code Quality / Duplication

6. RBAC permission matrix is copy-paste instead of data-driven

Six nearly identical test.describe.serial blocks (Global RW, Global RO, Global Denied, Individual Denied, Individual RW, Individual RO) follow the exact same pattern — only the role name, policy specs, and expected UI outcome differ. This is a textbook case for parameterized testing.

The Notifications workspace already demonstrates this pattern with severity filtering. After moving setup to beforeAll, this could be:

const rbacScenarios = [
  {
    name: "Global Read-Write",
    roleName: "role:default/workflowReadwrite",
    policies: globalWorkflowPolicies("allow", "allow"),
    expectWorkflowVisible: true,
    expectRunEnabled: true,
  },
  {
    name: "Global Read-Only",
    roleName: "role:default/workflowReadonly",
    policies: globalWorkflowPolicies("allow", "deny"),
    expectWorkflowVisible: true,
    expectRunEnabled: false,
  },
  {
    name: "Global Denied",
    roleName: "role:default/workflowDenied",
    policies: globalWorkflowPolicies("deny", "deny"),
    expectWorkflowVisible: false,
    expectRunEnabled: false,
  },
  // ... Individual Denied, Individual RW, Individual RO
];

for (const scenario of rbacScenarios) {
  test.describe(`RBAC: ${scenario.name}`, () => {
    test.beforeAll(async () => {
      await createRoleWithPolicies(apiToken, scenario.roleName, [PRIMARY_USER], scenario.policies);
    });
    test.afterAll(async () => {
      await deleteRoleAndPolicies(apiToken, scenario.roleName);
    });

    test(`Verify ${scenario.name} access`, async ({ page, uiHelper }) => {
      await orchestratorPO.navigateToOrchestrator();
      if (scenario.expectWorkflowVisible) {
        await orchestratorPO.verifyWorkflowVisible("Greeting workflow");
        await orchestratorPO.verifyRunButtonState(
          scenario.expectRunEnabled ? "enabled" : "disabled"
        );
      } else {
        await orchestratorPO.verifyWorkflowNotVisible("Greeting workflow");
      }
    });
  });
}

~375 lines → ~50 lines. The permission matrix is immediately visible.

7. Run button verification pattern duplicated with identical logic

Appears in at least 3 places with the same code:

const runButton = page.getByRole("button", { name: "Run" });
const buttonCount = await runButton.count();
if (buttonCount === 0) {
  expect(buttonCount).toBe(0);
} else {
  await expect(runButton).toBeDisabled();
}

Should be a single page object method: verifyRunButtonState('enabled' | 'disabled' | 'absent').

8. 26 eslint-disable comments

Many are for playwright/no-conditional-in-test and playwright/no-conditional-expect. While some conditionals are genuinely necessary, the volume suggests the test logic needs restructuring. Notable examples:

• orchestrator-rbac.tests.ts:646-648 — Conditional breadcrumb navigation that silently passes if no breadcrumb exists. This should be a clear assertion.
• orchestrator.tests.ts:460-463 — Early return based on IS_OPENSHIFT skips assertions without test.skip().

9. Hardcoded timeouts as magic numbers

Timeouts are scattered as raw numbers: 150_000, 180_000, 240_000, 30000, 10000, 15000, 120000, 5000. The repo pattern uses test.setTimeout() at the describe level. Named constants would improve readability:

const WORKFLOW_EXECUTION_TIMEOUT = 150_000;
const ELEMENT_VISIBILITY_TIMEOUT = 30_000;

10. page.waitForTimeout() hardcoded sleeps

• orchestrator-rbac.tests.ts:719 — await page.waitForTimeout(2000) after creating a role
• orchestrator-rbac.tests.ts:919 — await page.waitForTimeout(10000) waiting for error indicators

These should use proper Playwright waiting patterns (retry assertions, waitFor, expect().toBeVisible()).

11. Functions marked async with no await

patchHttpbin() (line 802), restartAndWait() (line 847), and cleanupAfterTest() (line 869) in orchestrator.tests.ts are async but only use synchronous execFileSync. The async keyword is misleading.

LOW — Style / Minor

12. Infrastructure functions defined at the bottom of spec file

orchestrator.tests.ts lines 780-878 (getHttpbinValue, patchHttpbin, restartAndWait, cleanupAfterTest) are cluster manipulation utilities. They belong in workflow-deployment-helpers.ts or support/utils/, not in a spec file.

13. Unused destructured fixture page: _page

Several tests destructure page as _page:

test("Greeting workflow execution...", async ({ uiHelper, page: _page }) => {

If page isn't needed, don't destructure it.

14. Test names are overly verbose

Some read like paragraphs:

• "Verify individual workflow read-only access - only Greeting workflow visible, Run button disabled"
• "Create workflow admin role and update secondary user membership"

The repo convention is concise: "Check Administration side nav has RBAC plugin". Consider shortening.

15. Fragile error detection in RHIDP-11839

Lines 921-952 check for errors by scanning for generic keywords (unauthorized, denied, permission, forbidden, failed). The word "failed" can appear in many unrelated contexts. A more targeted assertion against a specific error message would be more reliable.

Positive Observations

• test-helpers.ts is well-structured with reusable RBAC utilities (createRoleWithPolicies, verifyRoleWithPolicies, deleteRoleAndPolicies)
• workflow-deployment-helpers.ts is thorough — operator version detection, image alignment, Postgres patching, retry logic
• Good use of test.runOnce() for expensive infrastructure deployment
• Proper cleanup in afterAll blocks
• ensureDataIndexOrSkip is a smart resilience pattern for flaky data-index
• Security-conscious namespace validation before shell command execution (line 477)
• Good use of OrchestratorPage from shared test utils where available

Suggested file structure after refactoring

workspaces/orchestrator/e2e-tests/
├── playwright.config.ts
├── package.json
└── tests/
    ├── config/
    │   ├── app-config-rhdh.yaml
    │   └── rhdh-secrets.yaml
    ├── support/
    │   ├── pages/
    │   │   └── orchestrator-po.ts       (~130 lines)
    │   └── utils/
    │       ├── deployment-helpers.ts     (~700 lines, renamed)
    │       └── cluster-helpers.ts        (patchHttpbin, restartAndWait, etc.)
    └── specs/
        ├── orchestrator.spec.ts          (~780 lines, or split via fallback)
        └── test-helpers.ts              (~420 lines, RBAC + auth utilities)